fix: fmt cannot accept multiple fns#327
Conversation
jrycw
changed the title
fmt cannot accept multiple fnsfmt cannot accept multiple fns
machow
left a comment
machow
left a comment
There was a problem hiding this comment.
Hey @jrycw, thanks for taking a look at this aspect of formatting. I left a comment explaining a bit more, but I think a list of formatting functions isn't expected (but an unfortunate bit of documentation copied from R made it sound like the case
It seems like some of the failing snapshots have changed because their formatting isn't running anymore, but I'm not sure why (based on the new code). For example,
def test_html_string_generated(gt_tbl: GT, snapshot: str):
> assert snapshot == gt_tbl.as_raw_html()
E assert [- snapshot] == [+ received]
E '''
E ...
E <td class="gt_row gt_left">apricot</td>
E - <td class="gt_row gt_right">$49.95</td>
E + <td class="gt_row gt_right">49.95</td>
E </tr>
E ...
E <td class="gt_row gt_left">banana</td>
E - <td class="gt_row gt_right">$17.95</td>
E + <td class="gt_row gt_right">17.95</td>
E </tr>
In this output, it seems like the dollar signs are expected, since it runs the fmt_currency() method.
| # if this is violated, get_loc will return a mask | ||
| col_indx = data.columns.get_loc(column) | ||
| data.iloc[row, col_indx] = value | ||
| return data |
There was a problem hiding this comment.
Since _set_cell() mutates data, it seems useful to continue not returning data (sometimes called Command query separation).
| formatter = FormatInfo(fns, col_res, row_pos) | ||
| # If a single function is supplied to `fns` then | ||
| # repackage that into a list as the `default` function | ||
| formatter: list[FormatInfo] = [] |
There was a problem hiding this comment.
I think unfortunately GT.fmt never expects a list of functions as inputs, but uses the maybe trickily named FormatFns to mean, a dataclass that holds functions for different renderers (e.g. html, in the future latex, etc..).
It's a good point though that GT.fmt is an important entry point, so it's worth thinking carefully about its behavior. I think if it comes down to it, people could always pass lambda x: formatter2(formatter1(x))? Or use some implementation of a compose() function?
These docs:
Parameters
----------
fns
Either a single formatting function or a named list of functions.
Unfortunately are copied over from R, and so named list is referring to something more like a dictionary (which we're currently using the FormatFns dataclass for)
There was a problem hiding this comment.
I'm curious about the possibility of implementing a compose() function. In my imagination, the code might look something like this:
from functools import partial
from typing import Any, Callable
from great_tables import GT, exibble
def compose(funcs: list[Callable[Any, Any]]) -> Callable:
def _compose(x: Any, funcs: list[Callable[Any, Any]]) -> Any:
for f in funcs:
x = f(x)
return x
return partial(_compose, funcs=funcs)
(
GT(exibble[["fctr"]]).fmt(
compose([lambda x: f"_{x}_", lambda x: f"*{x}*", lambda x: f"<{x}>"]),
columns="fctr",
)
)
|
@machow , thank you so much for explaining the details to me. I believe making a small modification to the docs will make it easier for people who haven't used |
That makes a ton of sense --- I can make sure to change the docstring Monday. I'll look back through to see if there are pieces to keep from the PR (since it seemed like there were some other improvements in here :) |
|
@machow, thanks a lot. |
|
@machow, this PR has been open for a while. Could you please help edit the docstring, and then we might be able to close it? |
|
@rich-iannone let's keep fmt()'s current behavior. Do you mind checking to make sure any of the docstring corrections are applied (can close this and open a new PR is useful). |
|
I'm closing this in favor of fixing the misleading documentation in the new PR #818. Thanks! |
3 participants
Currently , the following code will raise
AttributeError: 'list' object has no attribute 'default'.The main issue appears to be that
fmtdid not handlefnsproperly when it is alist. As a result, the callables are not wrapped inFormatFns. Additionally,Body.render_formatsneeds to be refactored to accommodate this change by retrieving the result of_set_cellfor each call. Given the critical role offmtwithin this project, while the proposed PR may not be flawless, it serves as a reference for our refactoring efforts. With some snapshot tests failing, I would greatly appreciate the team's assistance in reviewing the PR to ensure the failed tests are acceptable.